[resubmission] move NXpid_controller to base_classes #1528
Conversation
lukaspie
added
the
NIAC vote needed
lukaspie
changed the title
|
Telco: it would be nice if this had a bit of clarification at the top. @benajamin volunteered to do this. Once done, ok to vote. |
lukaspie
changed the title
|
@benajamin thanks for volunteering on this, please let me how I can help you. Please note that we renamed the class now to |
|
@benajamin substantially added to the description of PID controllers in this PR, it should now be ready for review. |
lukaspie
force-pushed
the
nxpid
branch
2 times, most recently
from
ce82b0b to
b979d33
Compare
|
Thanks @benajamin for the new text |
|
Hi all, as discussed in the Telco, we can now move this PR to an online vote. NIAC committee members please vote on this PR using emojis on this comment. 👍 for yes, 👎 for no, anything else (for example 👀) to abstain. We need 14 votes to hit quorum so please review and vote! |
| It can also be a link to an ``NXsensor.value`` field. | ||
| </doc> | ||
| </field> | ||
| <field name="K_p_value" type="NX_NUMBER"> |
There was a problem hiding this comment.
Seems a little odd ending the field names with _value; for example, using K_p instead of K_p_value.
There was a problem hiding this comment.
Would be okay for me, but not sure if covered by the vote here. Maybe could just be done after the vote is in and before the PR is merged.
There was a problem hiding this comment.
OK with me.
Also we might like to capture some guidelines when naming of fields and groups. I don't think we have these at the moment.
This is a follow-up to #1414 where NXpid was mistakenly not included in the vote even though it was originally used within NXactuator. This PR moves NXpid to the base classes and adds it back to NXactuator.
This is a resubmission of #1522 which was marked as merged by GitHub after merging #1414 even though NXpid is not actually part of the
base_classesyet.